feat: contribute aws ec2 resource detector #1455
Conversation
yiyuan-he
requested review from
a team,
ahayworth,
arielvalentin,
dazuma,
ericmustin,
fbogsany,
kaylareopelle,
mwear,
plantfansam,
robbkidd,
robertlaurin,
simi and
xuan-cao-swi
as code owners
yiyuan-he
changed the title
yiyuan-he
changed the title
yiyuan-he
changed the title
yiyuan-he
changed the title
yiyuan-he
force-pushed
the
contribute-aws-resource-detectors
branch
from
b3104e3 to
2ec79b1
Compare
yiyuan-he
changed the title
yiyuan-he
force-pushed
the
contribute-aws-resource-detectors
branch
from
2ec79b1 to
1a699b9
Compare
yiyuan-he
force-pushed
the
contribute-aws-resource-detectors
branch
from
d9331bb to
5335cbe
Compare
|
LGTM. Please fix the ci. |
| http.read_timeout = HTTP_TIMEOUT | ||
|
|
||
| begin | ||
| http.request(request) |
There was a problem hiding this comment.
Add OpenTelemetry::Common::Utilities.untraced around http.request to avoid random span from detection.
|
Thanks for reviewing @xuan-cao-swi - I'll push some commits to address your comments shortly. I was also wondering if you or any of the other reviewers would be willing to sponsor my membership to the OTel org. More context in this comment from another PR of mine. |
yiyuan-he
force-pushed
the
contribute-aws-resource-detectors
branch
from
1b1db7d to
f11d75d
Compare
| begin | ||
| # Get IMDSv2 token - this will fail quickly if not on EC2 | ||
| token = fetch_token | ||
| return OpenTelemetry::SDK::Resources::Resource.create({}) if token.nil? |
There was a problem hiding this comment.
For IMDSv1, token is not required to retrieve metadata. I think it's better to allow nil token. If somehow with IMDSv2, the token is still nil, then let it fail.
opentelemetry-js seems allow both token and identity to be nil/empty (but just make sure identity = fetch_identity_document(token) || {})
There was a problem hiding this comment.
Thanks Xuan - Updated the implementation and unit tests to support both IMDSv1 and IMDSv2 cases.
|
@yiyuan-he , yes, I am happy to sponsor you. Just left one comment about IMDSv1 vs IMDSv2. |
kaylareopelle
left a comment
kaylareopelle
left a comment
There was a problem hiding this comment.
Thank you, @yiyuan-he! Few small things, but overall this looks good.
resources/aws/lib/opentelemetry/resource/detector/aws/version.rb
Outdated
Show resolved
Hide resolved
|
Thanks @kaylareopelle! Updated the versions and made a constant for that long OTel prefix. |
kaylareopelle
left a comment
kaylareopelle
left a comment
There was a problem hiding this comment.
Thanks for the updates!
3 participants
What does this pull request do?
Sets up a new AWS resource detector gem and adds functionality for EC2 resource detection. The implementation is similar to those that already exist for Python and JavaScript.
I will also raise PRs to add resource detection for other platforms such as ECS, Lambda, and EKS in the future.
Related Issues
#34